Skip to content

eth/gasprice: implement feeHistory API#23033

Merged
zsfelfoldi merged 10 commits intoethereum:masterfrom
zsfelfoldi:feehistory
Jun 28, 2021
Merged

eth/gasprice: implement feeHistory API#23033
zsfelfoldi merged 10 commits intoethereum:masterfrom
zsfelfoldi:feehistory

Conversation

@zsfelfoldi
Copy link
Copy Markdown
Contributor

@zsfelfoldi zsfelfoldi commented Jun 11, 2021

This PR implements the feeHistory EIP-1559 gas price API specified here:
ethereum/execution-specs#212
Original proposal:
https://gist.github.com/zsfelfoldi/473e29106d38525de6b4413e2ebcddf1

@zsfelfoldi zsfelfoldi changed the title eth/gasprice: implement feeHistory API (WIP) eth/gasprice: implement feeHistory API Jun 16, 2021
@zsfelfoldi zsfelfoldi changed the title eth/gasprice: implement feeHistory API eth/gasprice: implement feeHistory API (WIP) Jun 17, 2021
@zsfelfoldi zsfelfoldi force-pushed the feehistory branch 2 times, most recently from 8969be1 to 1f6ca7b Compare June 18, 2021 04:01
@zsfelfoldi zsfelfoldi changed the title eth/gasprice: implement feeHistory API (WIP) eth/gasprice: implement feeHistory API Jun 18, 2021
Comment thread eth/gasprice/feehistory.go Outdated
Comment thread eth/gasprice/feehistory.go Outdated
Comment thread eth/gasprice/feehistory.go Outdated
Comment thread eth/gasprice/feehistory.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can simplify the thing like this

func (f *Oracle) resolveLastBlockNumber(last rpc.BlockNumber) (*types.Block, uint64, bool, error) {
	var (
		pending *types.Block
		noHead  bool
	)
	if last == rpc.PendingBlockNumber {
		pending, _ = f.backend.BlockByNumber(ctx, last)
		if pending != nil {
			return pending, pending.NumberU64(), false, nil
		}
		last, noHead = rpc.LatestBlockNumber, true
	}
	if last == rpc.LatestBlockNumber {
		latestHeader, err := f.backend.HeaderByNumber(ctx, rpc.LatestBlockNumber)
		if err == nil {
			last = rpc.BlockNumber(latestHeader.Number.Uint64())
			return nil, last, noHead, nil
		} 
		return nil, 0, false, err
	}
	return nil, last, false, nil
}

The headBlockNumber actually is unnecessary. If pending is not nil and it's requested for processing later, we can use the pending field here for checking, instead of using headBlockNumber

And also the latestHeader is also kind of unnecessary since retrieve the header again is cheap because of the cache.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And we can call this function like this. It's much cleaner.

pending, last, noHead, err := f.resolveLastBlockNumber(lastBlockNumber)
	if err != nil {
		return 0, nil, nil, nil, err
	}
	if noHead {
		blockCount--
		if blockCount == 0 {
			return
		}
	}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I do need headBlockNumber to enforce maxHistory and avoid requesting future blocks. But factoring out the block number resolving logic is a great idea, I think it's a lot more readable now. Also you're right about latestHeader, saving it is not really imporant and not doing so makes the code simpler.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The headBlockNumber is last returned by resolveLastBlockNumber

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not true if lastBlockNumber was an explicit block number different from the head. We still need the head though to apply maxHistory restriction and prevent future block requests. So I think it's better to also integrate these checks into resolveBlockRange that can also limit blockCount if necessary.

Comment thread eth/gasprice/feehistory.go Outdated
Comment thread eth/gasprice/feehistory.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we reject this request in the first place? I don't think request the transaction fees of the future blocks make any sense.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, it's probably better to reject it explicitly. Now I return an error in this case.

Comment thread internal/ethapi/api.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use the hexutil.Big?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to but then forgot about it. Did it now.

)
if pendingBlock, pendingReceipts, lastBlockNumber, blockCount, err = oracle.resolveBlockRange(ctx, lastBlockNumber, blockCount, maxHistory); err != nil {
return
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to return directly if the block count is 0 here. It's cleaner

JukLee0ira added a commit to JukLee0ira/XDPoSChain that referenced this pull request Jul 12, 2024
JukLee0ira added a commit to JukLee0ira/XDPoSChain that referenced this pull request Aug 3, 2024
gzliudan pushed a commit to XinFinOrg/XDPoSChain that referenced this pull request Aug 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants